fix: make the redis reply buffer and the http/2 request head linear - #687
Merged
Conversation
…uffer std.redis buffered every reply by appending to an immutable string, so each 4 KiB read copied everything read so far. The reply size a server may advertise is bounded at MAX_BULK_LEN, 512 MiB, which is about 131,000 appends averaging 256 MiB each: a bulk reply large enough is not slow, it never returns. Every RESP reply went through that one path. The buffer is now bytes plus a read cursor. Consuming a line or a payload moves the cursor, and the consumed prefix is dropped in one step when the next read arrives, so no byte is copied twice on the way through. Line scanning resumes where the previous scan stopped, one byte back so a CR at a read boundary still pairs with its LF. A bulk read knows the length up front, so it reads the remainder straight into a growing buffer and asks the socket for exactly what is left, which also means nothing is read past the payload. Reading bytes rather than a lossily-decoded string fixes two things on the way. A payload containing a NUL byte used to shorten the buffer the client measured its progress against, so the client waited for bytes that had already arrived; that stream now stays in sync. And bytes that are not utf-8 surface as an error instead of a string silently filled with replacement characters. The up-front reservation for a bulk read is capped, so a twelve-byte header advertising 512 MiB cannot turn into a 512 MiB allocation before any payload byte has arrived.
The http/2 server bridges a decoded header list into an http/1.1 request head, and it did that with `head = head + name + ": " + value + crlf` — four appends per field, each copying everything written before it. open_stream has already capped the list at MAX_HEADER_COUNT fields and MAX_HEADER_LIST_BYTES, so the worst request that reaches the bridge is bounded, and that bound is what keeps this from being an availability bug. It still means about twelve megabytes of copying to produce a head of sixty kilobytes, and it is paid on every request. The head now goes into a byte buffer, which build_http_request wanted as bytes anyway, so the final string-to-bytes conversion goes with it. Appending an empty string to a ByteBuffer was an error rather than a no-op: write_string_utf8 returns the number of bytes written and the result convention reads zero as failure. ByteBuffer.write already short-circuits an empty write for exactly this reason and write_string_utf8 did not, which made a header sent with an empty value — legal, and common enough — drop the whole request. It now short-circuits the same way, with a test on each side of the boundary.
kacy
force-pushed
the
perf-linear-string-accumulation
branch
from
August 9, 2026 14:30
6c6b3b9 to
148ffb8
Compare
A socket read allocates and zeroes everything it asks for, so one read size for both jobs is the wrong trade: a 64 KiB read makes every "+PONG" pay for the one large bulk reply. Measured over 20,000 pings on one connection that cost about 5%. The line path is back to a 4 KiB read, matching what it always asked for, and the bulk gather — where the length is known and a read never over-reads — keeps the 64 KiB read that makes a large reply cheap. Small replies are level with the old client again and a 64 MiB bulk still lands in under 250 ms.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two places in std grew a string by appending to it in a loop. Pith strings are
immutable, so each append allocates a fresh string and copies everything written
so far. One of the two is an availability bug; the other is a bounded cost paid
on a hot path, and it turned out to be smaller than the report suggested.
the redis client could not read a large reply
std.redisbuffered every reply in a string and appended each 4 KiB socket readto it. A server may advertise a bulk string up to
MAX_BULK_LEN, 512 MiB, whichis roughly 131,000 appends averaging 256 MiB of copying each. That is not a slow
read, it is a read that does not come back, and
fillwas the only bufferingpath in the client, so every RESP reply went through it. Measured: 1 MiB of bulk
reply took 13 ms, 4 MiB took 126 ms, 16 MiB took 2.8 s, and 64 MiB had not
finished after 55 minutes.
The consume side mattered as much as the append side. The buffer was also read
from the front —
read_linesliced the line off and re-assigned the remainder —so an array reply of many small elements copied the whole tail once per element,
independent of the append cost. A buffer that appends cheaply but re-slices on
every consume just moves the work. So the buffer is now bytes plus a read
cursor: consuming moves the cursor, and the consumed prefix is dropped in one
step when the next read arrives. Line scanning resumes where the previous scan
stopped, one byte back so a CR at a read boundary still pairs with its LF, which
means every byte is examined once however the reply is split. A bulk read knows
its length up front, so it reads the remainder straight into a growing buffer
and asks the socket for exactly what is left — which also means nothing is ever
read past the payload.
Reading bytes instead of a lossily-decoded string fixes two things on the way. A
payload containing a NUL byte used to shorten the buffer the client measured its
own progress against, so it waited for bytes that had already arrived and the
connection wedged; the stream now stays in sync. And a bulk string that is not
valid utf-8 surfaces as an error rather than as a string quietly filled with
replacement characters. That last one is a visible behaviour change, and it is
noted in the module header.
The up-front reservation for a bulk read is capped, so a twelve-byte header
advertising 512 MiB cannot turn into a 512 MiB allocation before a single
payload byte has arrived. Growing the buffer as data lands keeps the old
resistance to a server that lies about a length.
The two reads want different sizes. A socket read allocates and zeroes
everything it asks for, so a single large read size would make every "+PONG"
pay for the one large bulk reply — measured at about 5% over 20,000 pings. The
line path asks for 4 KiB, which is what it always asked for; the bulk gather,
where the length is known and a read can never over-read, asks for 64 KiB and
that is what makes a large reply cheap.
the http/2 request bridge is bounded, and smaller than it looked
build_requestsynthesizes an http/1.1 request head from the decoded headerlist with four appends per field. The report described this as an unbounded
site, then corrected it to bounded by
MAX_HEADER_BLOCK_BYTESat about 1000fields. Neither is right, and the real bound is tighter than both:
MAX_HEADER_BLOCK_BYTEScaps the compressed block, and hpack indexing means80 KB of block can carry tens of thousands of fields. What actually bounds this
is
header_list_within_limits, checked inopen_streambefore the bridge everruns:
MAX_HEADER_COUNTis 100 andMAX_HEADER_LIST_BYTESis 65536. So theworst request the server will accept costs about twelve megabytes of copying,
which measures as roughly 0.45 ms per request — real, paid on every request, but
not an availability bug. The head now goes into a byte buffer, which
build_http_requestwanted as bytes anyway, so the string-to-bytes conversionat the end goes with it.
That change needed a fix in
std.bytesto be correct.ByteBuffer.writetreatsan empty write as a no-op, because the builtin reports bytes written and the
result convention reads zero as failure;
write_string_utf8did not, soappending an empty string was an error. A header sent with an empty value is
legal, so without the fix the new bridge dropped any request carrying one — a
regression test drives exactly that request and it hangs with the fix reverted.
write_string_utf8now short-circuits the same waywritedoes.what was tested
Redis byte-for-byte equivalence against the old client. A new regression
case,
test_redis_split_reads, drives the client with a RESP script coveringstatus lines, error replies, positive and negative integers, bulk strings
including empty and null, empty and null arrays, a flat array mixing bulk,
integer and null elements, a nested array, and a 256 KiB bulk. It runs the same
script delivered whole, then one byte at a time, then in 3-byte and 7-byte
pieces, with a pause between pieces so the kernel does not coalesce them back
into a single read — the byte-at-a-time run puts a read boundary at every offset
in every reply, including inside every CRLF. It then checks the limits still
reject, each on its own connection:
MAX_BULK_LEN,MAX_ARRAY_ITEMS,MAX_REPLY_DEPTHandMAX_LINE_LEN, pinned by their error messages. Everysplit parses identically to the whole delivery, and the case's whole output is
byte-identical when run against the pre-change client — the old and new parsers
agree on all of it.
Redis timing. A loopback stub answering one GET with a bulk reply of a given
size, A/B interleaved, three runs each, medians:
Before is worse than quadratic in practice — 4x the size costs about 10x at
1 MiB and about 22x at 16 MiB, because each append is a fresh multi-megabyte
allocation on top of the copy. After is flat in bytes per second across the
whole range.
http/2 timing. 200 requests over one h2c connection, header count and value
size varied, A/B interleaved, three runs each. The largest row is the biggest
header list the server will accept:
That last row is 1.52 ms down to 1.06 ms per full request round trip, so about
30% off the end-to-end path at the cap and nothing measurable on ordinary
requests.
Empty header values. A second regression case,
test_http2_empty_header_value, sends a real h2c request carrying a header withan empty value, one with a value, and one more empty, and reports the head the
bridge synthesized. Header order is preserved, pseudo-headers are still skipped,
and the output is identical to the pre-change server. With the
std.bytesfixreverted and the bridge change kept, the same case hangs and produces nothing,
which is what the fix is there for.
The small-reply path did not regress. 20,000 pings on one connection, A/B
interleaved against the old client, three runs each: 1027/1062/1046 ms before
against 1024/1049/1059 ms after. With a single 64 KiB read size for both paths
this was 1072/1094/1105 ms, which is what split the constant in two.
No new leak, no invalid access.
test_redis_split_readsruns clean undervalgrind with
--error-exitcode=99, and its--leak-check=fullprofile isrecord-for-record identical between the old and new client — same counts, same
sizes, including the pre-existing blocks the harness itself leaks.
Corpus.
make run-regressions-onlyis 328/328 with the two new cases;examples/redis_client.pithstill runs.pith doc --checkpasses on all threechanged std files.